Skip to content

feat(ads): asset extension tools + RSA update + conversion-action mgmt#34

Open
illia-sapryga wants to merge 4 commits into
kLOsk:mainfrom
illia-sapryga:feat/asset-and-conversion-tools
Open

feat(ads): asset extension tools + RSA update + conversion-action mgmt#34
illia-sapryga wants to merge 4 commits into
kLOsk:mainfrom
illia-sapryga:feat/asset-and-conversion-tools

Conversation

@illia-sapryga
Copy link
Copy Markdown
Contributor

Summary

Three logical chunks bundled here because they share ads/write.py's dispatch + apply infrastructure and would conflict at the file level if split into separate PRs.

  1. Asset extension tools (call asset, location asset, ad schedule, geo exclusions, image assets, customer-scope refactor of callouts/sitelinks/snippets, promotion management, asset linking)
  2. RSA in-place update (update_responsive_search_ad)
  3. Conversion-action management (3 new tools — create / update / remove)

Depends on #33 (dynamic Google Ads enum introspection) — please merge that first.

What's added

1. Asset extension tools

Tool Purpose
draft_call_asset Campaign or customer scope, E.164 phone normalization, ad-schedule restriction, optional conversion-action override. Verified against BGI Tint (+19164609257).
draft_location_asset Google Business Profile-backed LOCATION_SYNC AssetSet, with label/listing filters
draft_image_assets Campaign image extensions from local files with MIME + dimension validation
draft_callouts Refactored to support BOTH campaign-scope AND customer-scope (the account-level CustomerAsset path that propagates to every eligible campaign automatically)
draft_sitelinks Same dual-scope refactor + URL reachability check + char-limit validation
draft_structured_snippets Same dual-scope refactor
add_ad_schedule Mon–Sat 8am–9pm-style scheduling via AdScheduleInfo CampaignCriterion
add_geo_exclusions Negative geo CampaignCriterion records to shrink a broad include list
link_asset_to_customer Promote existing Asset rows from campaign-scope to account-level (CustomerAsset) — for re-using shared logos / sitelinks across campaigns
update_call_asset / update_sitelink / update_callout In-place asset updates with FieldMask — only the fields the caller passes are modified
draft_promotion / update_promotion PromotionAsset create + swap (PromotionAsset is immutable, so 'update' is implemented as create-new-link-old-unlink atomically)
_apply_assets() Shared helper routing a populate-fn through either CampaignAsset or CustomerAsset linkage based on scope
_normalize_phone_e164() E.164 phone-number normalization with US/CA + EU trunk-prefix handling

2. RSA in-place update

Tool Purpose
update_responsive_search_ad Update existing RSAs without delete-then-recreate. Partial update via FieldMask — only the fields the caller passes are modified. Headlines/descriptions accept either bare strings (unpinned) or {\"text\": ..., \"pinned_field\": \"HEADLINE_1\"} dicts (pinned).

3. Conversion-action management (3 new tools)

Tool Purpose
draft_create_conversion_action Create AD_CALL / WEBSITE_CALL / WEBPAGE / WEBPAGE_CODELESS / GOOGLE_ANALYTICS_4_CUSTOM / etc. with value, threshold, attribution model, counting type
draft_update_conversion_action Partial update via FieldMask — rename / promote-to-Primary / demote-to-Secondary / set value / change duration threshold / change attribution
draft_remove_conversion_action Irreversible removal. Warns that SMART_CAMPAIGN_* and GOOGLE_HOSTED types reject mutation with MUTATE_NOT_ALLOWED (Google manages those).

The 3 conversion-action tools live in their own module adloop/ads/conversion_actions.py and route through _execute_plan's dispatch via lazy-import _apply_*_conversion_action_route shims.

Refactors (uses #33's enum_names())

The _VALID_TYPES, _VALID_CATEGORIES, _VALID_COUNTING_TYPES, _VALID_ATTRIBUTION_MODELS sets in conversion_actions.py are pulled dynamically from the SDK via enum_names(). Same for _VALID_PROMOTION_OCCASIONS, _VALID_DISCOUNT_MODIFIERS, _VALID_CALL_REPORTING_STATES in ads/write.py.

The hardcoded versions of those sets had drifted from the SDK. The ConversionActionTypeEnum set was missing 29 of 40 valid values; PromotionExtensionOccasionEnum was missing 2.

Validated end-to-end

This batch was used to build out a real Google Ads account end-to-end (BGI Tint, customer 8202753856):

  • Created 1 search campaign, 6 ad groups, 150 keywords, 6 RSAs, 22 account-level CustomerAssets (sitelinks/callouts/snippets/promotions/call/business logo), 20 campaign image extensions, 2 negative shared lists
  • Created 2 conversion actions (Calls from Ads (≥90s) AD_CALL @ $250, Website Call (GFN ≥90s) WEBSITE_CALL @ $250 with 90s threshold) and bound them to a custom conversion goal
  • Updated existing call asset to point at the new AD_CALL conversion with USE_RESOURCE_LEVEL_CALL_CONVERSION_ACTION
  • Updated 2 existing promotion assets in place

Test plan

  • Full suite passes: uv run pytest → 430/430 tests
  • tests/test_ads_extensions.py — comprehensive validation + apply-handler tests for every new function (uses fake services mirroring the google-ads SDK protos; no network)
  • tests/test_conversion_actions.py — 29 tests covering validation, partial update mask, FieldMask correctness, MCP registration
  • tests/test_update_rsa.py — RSA update integration tests
  • _apply_* handlers verified to emit the right FieldMask paths via fake service inspection
  • Promotion swap: verified that creating-new + linking-new + unlinking-old happens atomically against fake services

Known follow-ups (out of scope for this PR)

  • Older hardcoded enum sets in ads/write.py (_VALID_BIDDING_STRATEGIES, _VALID_CHANNEL_TYPES, _VALID_DAYS_OF_WEEK, _VALID_HEADLINE_PINS, etc.) could be migrated to enum_names() in a separate refactor PR.
  • update_call_asset allows full-list replacement of ad_schedule_targets but no append-mode — append would be useful for some workflows.

illia-sapryga added a commit to illia-sapryga/adloop that referenced this pull request May 6, 2026
The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions
and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules /
constants that don't exist in this PR. They live in the follow-up PR
(kLOsk#34, feat/asset-and-conversion-tools) and should be tested there.

Branch A is intentionally minimal: the helper + helper-only unit tests.

CI now passes — 12/12 tests.
kLOsk pushed a commit that referenced this pull request May 15, 2026
* feat(ads): dynamic Google Ads enum introspection helper

Adds adloop.ads.enums.enum_names() — pulls valid enum member names
straight from the google-ads SDK at the API version we're pinned to
(see adloop.ads.client.GOOGLE_ADS_API_VERSION).

Drops the need to hand-maintain parallel lists like:

    _VALID_CONVERSION_ACTION_TYPES = {"AD_CALL", "WEBSITE_CALL", ...}

— which otherwise drift every time the SDK or API version updates.
The helper is module-cached (functools.lru_cache) so the no-auth
GoogleAdsClient used for introspection is built once per process,
and every enum_names() call after the first is essentially free.

UNSPECIFIED + UNKNOWN sentinels are dropped by default since they
are protobuf zero-values that should never appear in user input.

Pure addition. No existing validators changed in this PR; downstream
PRs (conversion-action tools, in-place asset updates, promotion
helper refactors) will switch their hardcoded enum sets to enum_names()
calls in their own commits.

12 unit tests cover:
- enum_names returns a frozenset
- UNSPECIFIED + UNKNOWN are excluded by default and includable on opt-in
- Critical members (AD_CALL, WEBSITE_CALL, GOOGLE_SEARCH_ATTRIBUTION_DATA_DRIVEN,
  USE_RESOURCE_LEVEL_CALL_CONVERSION_ACTION, BLACK_FRIDAY) are present
- ConversionActionCountingTypeEnum returns exactly {ONE_PER_CLICK, MANY_PER_CLICK}
- Unknown enum names raise AttributeError
- LRU cache returns the same frozenset instance on repeat calls
- The introspection client is memoized

* test: remove cross-module enum tests from test_ads_enums.py

The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions
and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules /
constants that don't exist in this PR. They live in the follow-up PR
(#34, feat/asset-and-conversion-tools) and should be tested there.

Branch A is intentionally minimal: the helper + helper-only unit tests.

CI now passes — 12/12 tests.
illia-sapryga and others added 2 commits May 27, 2026 18:31
Adds the complete asset-extension and conversion-action management
surface that AdLoop was missing. Three logical chunks bundled here
because they share `ads/write.py`'s dispatch + apply infrastructure
and would conflict if split into separate PRs.

- `draft_call_asset` — campaign or customer scope, E.164 normalization,
  ad-schedule restriction, optional conversion-action override
- `draft_location_asset` — Google Business Profile-backed AssetSet
  (LOCATION_SYNC), with label/listing filters
- `draft_image_assets` — campaign image extensions from local files
  with MIME + dimension validation
- `draft_callouts`, `draft_structured_snippets`, `draft_sitelinks` —
  refactored to support BOTH campaign-scope AND customer-scope (the
  account-level CustomerAsset path that propagates to every eligible
  campaign automatically)
- `add_ad_schedule` — Mon-Sat 8am-9pm-style scheduling via
  AdScheduleInfo CampaignCriterion
- `add_geo_exclusions` — negative geo CampaignCriterion records to
  shrink a broad include list
- `_apply_assets()` shared helper routing a populate fn through either
  CampaignAsset or CustomerAsset linkage based on scope
- Phone-number E.164 normalization with US/CA + EU trunk-prefix handling

- Update existing RSAs without delete-then-recreate
- Partial update via FieldMask — only the fields the caller passes
  are modified
- Headlines/descriptions accept either bare strings (unpinned) or
  {"text": "...", "pinned_field": "HEADLINE_1"} dicts (pinned)

- `draft_create_conversion_action` — AD_CALL / WEBSITE_CALL / WEBPAGE
  / GA4_CUSTOM with value, threshold, attribution model, counting type
- `draft_update_conversion_action` — partial update with FieldMask;
  rename / promote-demote / set value / change duration threshold
- `draft_remove_conversion_action` — irreversible removal (warns that
  SMART_CAMPAIGN_* and GOOGLE_HOSTED types reject mutation)

The 3 conversion-action tools live in their own module
`adloop/ads/conversion_actions.py` and route through dispatch via
`_apply_*_conversion_action_route` shims kept in `ads/write.py`.

- `link_asset_to_customer` — promote existing Asset rows from
  campaign-scope to account-level (CustomerAsset)
- `update_call_asset` / `update_sitelink` / `update_callout` —
  in-place asset updates with FieldMask
- `draft_promotion` / `update_promotion` — PromotionAsset create
  + swap (PromotionAsset is immutable; update is implemented as
  create-new-link-old-unlink)
- Promotion module uses `enum_names("PromotionExtensionOccasionEnum")`
  + `enum_names("PromotionExtensionDiscountModifierEnum")` from
  the dynamic-enums helper
- Conversion-actions module uses `enum_names("...")` for all 4
  Google Ads enums it validates against — drops 4 hardcoded enum
  sets that were drifting from the SDK
- Auto-cleanup script `scripts/cleanup_sitelink_links.py` for
  duplicate sitelink CampaignAsset detection

- `tests/test_ads_extensions.py` — comprehensive validation +
  apply-handler tests for every new function (uses fake services
  mirroring the google-ads SDK protos; no network)
- `tests/test_conversion_actions.py` — 29 tests
- `tests/test_update_rsa.py` — RSA update integration tests
- All 430 tests pass
Add two new MCP tools for pushing offline conversion data directly to
Google Ads, complementing the existing conversion-action management:

- draft_upload_call_conversions: uploads call conversions (Caller ID +
  Call Start Time) to UPLOAD_CALLS-typed actions via
  ConversionUploadService.UploadCallConversions
- draft_upload_enhanced_conversions_for_leads: uploads hashed PII to
  UPLOAD_CLICKS-typed actions via UploadClickConversions with
  user_identifiers populated. Works retroactively — no "action must exist
  before the conversion" constraint that UPLOAD_CALLS has.

Both tools follow the AdLoop draft → confirm_and_apply pattern with
plan storage, dry-run, audit logging, and per-row error reporting via
partial_failure mode.

Adds 25 new unit tests covering CSV parsing, action-name resolution,
payload shape, partial-failure handling, and type-mismatch rejection
(UPLOAD_CALLS vs UPLOAD_CLICKS).

Also removes scripts/cleanup_sitelink_links.py, which was scoped to a
specific client cleanup and shouldn't ship in the public tool.
@illia-sapryga illia-sapryga force-pushed the feat/asset-and-conversion-tools branch from bacb626 to 7666a41 Compare May 27, 2026 23:35
@illia-sapryga
Copy link
Copy Markdown
Contributor Author

Hey @kLOsk, just rebased this onto main and resolved the conflicts (12 commits behind → caught up). Also extended the PR with two new conversion-upload tools — draft_upload_call_conversions + draft_upload_enhanced_conversions_for_leads — that follow the existing draft → confirm_and_apply pattern. They're the "import conversions" complement to the conversion-action management already in this PR. 517 tests passing, MERGEABLE/CLEAN.

If you'd prefer those upload tools split into a separate PR for easier review, happy to break them out. Otherwise, anything else you'd want before merging?

…es on create

Two issues surfaced when creating ConversionActions via the Google Ads
API that were not covered by validation:

1. include_in_conversions_metric is IMMUTABLE on create — Google derives
   it from the conversion category and rejects any value supplied in the
   create mutate with IMMUTABLE_FIELD. The apply function previously
   set it unconditionally, blocking every AD_CALL / PHONE_CALL_LEAD
   create with the default True value. Removed the line; callers who
   need to change it use draft_update_conversion_action after create.

2. default_value > 0 paired with always_use_default_value=False is
   rejected as INVALID_VALUE — it reads as "you set a value but told me
   not to use it". The draft tool defaults always_use_default_value to
   False, so the typical "set a default $250 lead value" call kept
   failing with no useful error. Added an auto-correct in the draft
   function: when default_value > 0 and the caller didn't explicitly
   opt out, flip always_use_default_value to True.

Tests added cover both regressions plus a sanity test that callers
who already set the flag correctly still get their value through, and
that default_value=0 keeps the flag False (for snippet-value callers).

All 227 tests pass.
draft_call_asset previously supported only customer or campaign scope.
This blocks the multi-service / multi-ad-group pattern where each ad
group inside one campaign needs its own tracked phone number wired to
its own service-specific call conversion action (e.g. Tint ad group
fires "Tint Call from Ad" worth $400, PPF ad group fires "PPF Call
from Ad" worth $2,000).

Added an ad_group_id parameter to both the MCP wrapper in server.py
and the impl in ads/write.py, with most-specific-wins scope precedence
(ad_group > campaign > customer). When ad_group_id is provided, the
apply function creates an AdGroupAsset link instead of a CampaignAsset
or CustomerAsset link.

Tests added:
  - draft test: ad_group_id selects ad_group scope and populates entity
  - draft test: ad_group_id wins over campaign_id (precedence rule)
  - apply test: creates an AdGroupAsset link with correct ad_group path
    and conversion action wiring
  - apply test: missing ad_group_id raises a clear error

Also fixed a latent bug in _FakeGoogleAdsService.ad_group_path: it
inherited from _FakePathService which uses a single prefix
("campaigns") for all resource path helpers, so ad_group_path returned
"customers/.../campaigns/{id}" instead of "customers/.../adGroups/{id}".
Override returns the correct prefix; only affects the new ad-group
scope test, no existing tests touched ad_group_path.

NOTE: when adding new params to MCP tools you must update BOTH the
impl in ads/*.py AND the wrapper in server.py — the wrapper's signature
is what FastMCP uses to build the JSON schema exposed to clients. The
first patch attempt only updated the impl and broke at call time with
unexpected_keyword_argument.

All 227 tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant